PDF-2231: Per-page multi-page TIFF (mixed orientation) + resolution/Rgb24 writer fixes#150
PDF-2231: Per-page multi-page TIFF (mixed orientation) + resolution/Rgb24 writer fixes#150Sawraz-IS wants to merge 4 commits into
Conversation
mee-ironsoftware
left a comment
There was a problem hiding this comment.
Approve with comments. Core mixed-orientation fix is correct, tested, and lives at the right layer (Drawing, so other products inherit it) — nothing blocking. Two Medium follow-ups recommended before merge (inline). Note: companion IronPdf #956 reviewed alongside — its -ci/prerelease pins are fine on a develop base, but PR #956's integration tests assert orientation only, so the per-page DPI round-trip is unverified end-to-end (see Medium on the resolution line here).
| case SixLabors.ImageSharp.Metadata.PixelResolutionUnit.PixelsPerMeter: | ||
| output.SetField(TiffTag.XRESOLUTION, image.Metadata.HorizontalResolution * 100); | ||
| output.SetField(TiffTag.YRESOLUTION, image.Metadata.VerticalResolution * 100); | ||
| output.SetField(TiffTag.XRESOLUTION, image.Metadata.HorizontalResolution / 100); |
There was a problem hiding this comment.
Medium: This / 100 fix is correct (TIFF has no per-metre unit; with RESOLUTIONUNIT = CENTIMETER, px/m / 100 = px/cm — the old * 100 wrote a value ~10^4x too large). But no test actually exercises this branch: CreateMultiFrameTiffBytes_Preserves_Per_Page_Dimensions_And_Resolution builds its bitmaps with PixelsPerInch, so it runs the unchanged Inch branch above. The PR description's claim that that test "proves the unit-conversion fix" isn't accurate, and the fix has no regression guard. Please add a case that sets PixelsPerMeter on the source and asserts the read-back DPI via the existing ToDotsPerInch helper — that test would fail under the old * 100.
There was a problem hiding this comment.
Added CreateMultiFrameTiffBytes_PreservesResolution_FromPixelsPerMeterSource, which builds a source with PixelResolutionUnit.PixelsPerMeter (300 DPI expressed as px/m), runs it through CreateMultiFrameTiffBytes, and asserts the read-back resolution via ToDotsPerInch ≈ 300. I verified it fails under the old * 100 (got 2999994).
| // Bgr24 is likewise 3 bytes/pixel; convert to Rgba32 for the | ||
| // same reason as Rgb24 above (this also yields the correct | ||
| // R,G,B sample order under PHOTOMETRIC.RGB). | ||
| using var rgba = imageAsFormat.CloneAs<Rgba32>(); |
There was a problem hiding this comment.
Low/Nit: Good fix here (and on the Rgb24 case) — the 3-bpp source packed into the 4-bpp buffer was misaligning every scanline, and CloneAs<Rgba32> also gives the correct R,G,B sample order. Out-of-scope follow-up: the sibling Abgr32 / Argb32 / Bgra32 cases just below still CopyPixelDataTo directly, but the writer emits PHOTOMETRIC.RGB with sample order R,G,B,A — so e.g. Bgra32 (bytes B,G,R,A) would land channel-swapped if ever hit. Pre-existing and the new public API avoids it entirely (it CloneAs<Rgba32> upfront), but worth a ticket to normalise these or confirm they're unreachable.
There was a problem hiding this comment.
Normalized all three to CloneAs<Rgba32>() (matching the Rgb24/Bgr24 pattern), so the writer always gets correct R,G,B,A order under PHOTOMETRIC.RGB.
| if (images == null) | ||
| throw new ArgumentNullException(nameof(images)); | ||
|
|
||
| var frames = new List<Image>(); |
There was a problem hiding this comment.
Low: Nice touch forcing every frame through CloneAs<Rgba32> so the writer's default (correct-channel-order) branch is always taken — a clean correctness firewall. One memory note: the clones are eagerly materialised into this List<Image> before any write, so for a moment both the caller's pages and the full set of Rgba32 clones are alive (transient ~2x page memory). Fine for typical use and no worse than the old IronPdf path, but for very large / high-DPI documents a lazy Select(... CloneAs<Rgba32> ...) with per-frame dispose after WriteDirectory would lower the peak. Not blocking.
There was a problem hiding this comment.
The suggested lazy + per-frame-dispose can't be done safely in InternalSaveAsMultiPageTiff because it's shared with CreateMultiFrameTiff(IEnumerable<AnyBitmap>) / (IEnumerable<string>), which pass borrowed images (the (Image)anyBitmap cast returns the AnyBitmap's internal frame), disposing those inside the writer would corrupt the source bitmaps. Intentionally left as-is.
|
|
||
| foreach (var page in pages) | ||
| { | ||
| ToDotsPerInch(page.XResolution, page.ResolutionUnit) |
There was a problem hiding this comment.
Low/Nit: This DPI assertion only ever sees the INCH branch because CreateSolidBitmap sets PixelsPerInch. The ToDotsPerInch helper already handles CENTIMETER, so adding a PixelsPerMeter source here would extend this same test to cover the actual resolution fix (see the Medium on AnyBitmap.cs).
0cf21c5 to
9abef96
Compare
Description
Adds per-page multi-page TIFF support to
AnyBitmapand fixes two latent bugs in the existing multi-page TIFF writer (InternalSaveAsMultiPageTiff).This is the IronSoftware.Drawing side of PDF-2231:
PdfDocument.ToMultiPageTiffStream()(in IronPdf) normalized every page of a mixed-orientation PDF to a single uniform size/orientation. The root cause is that a multi-page TIFF was being assembled as a single ImageSharpImage<T>, which requires all frames to share one size. This PR provides a path that keeps each page's native dimensions/orientation/resolution.New public APIs:
AnyBitmap.CreateMultiFrameTiffStream(IEnumerable<AnyBitmap>)→MemoryStreamAnyBitmap.CreateMultiFrameTiffBytes(IEnumerable<AnyBitmap>)→byte[]Both reuse the existing per-page LibTiff writer (one IFD per page) instead of round-tripping through a single ImageSharp image (the lossy step in the old
CreateMultiFrameTiff/FromStreampath). Each page is converted toRgba32first so its byte layout matches the writer regardless of source format.Bug fixes in
InternalSaveAsMultiPageTiff:PixelsPerMeterbranch multiplied by 100 when converting pixels-per-metre → pixels-per-centimetre; it must divide by 100. TIFF has no per-metre unit, so the value was previously written ~10,000× too large.Rgb24/Bgr243-bpp stride corruption: The writer fills a 4-samples/pixel (RGBA) buffer, but 3-byteRgb24/Bgr24pixels were copied tightly-packed, shifting every row and scrambling the page. These cases now convert toRgba32before copying.Type of change
How Has This Been Tested?
Added unit tests in
AnyBitmapFunctionality.cs:CreateMultiFrameTiffStream_Preserves_Mixed_Orientation: Portrait + landscape pages keep their own dimensions (read back per-IFD with LibTiff).CreateMultiFrameTiffBytes_Preserves_Per_Page_Dimensions_And_Resolution: Per-page sizes preserved and DPI round-trips to the source value (proves the unit-conversion fix).CreateMultiFrameTiffStream_Empty_Sequence_Throws: Argument validation.CreateMultiFrameTiff_Preserves_Rgb24_Pixels: A JPEG (decodes toRgb24) round-trips through the writer with sampled pixels matching the source (proves the 3-bpp stride fix). Verified this test fails with the fix reverted.Checklist: